Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DVT-1057 replace block data structure with LRU cache to fix memory leak #148

Merged
merged 24 commits into from
Nov 13, 2023
Merged

DVT-1057 replace block data structure with LRU cache to fix memory leak #148

merged 24 commits into from
Nov 13, 2023

Conversation

gatsbyz
Copy link
Contributor

@gatsbyz gatsbyz commented Nov 2, 2023 •
edited
Loading

  • implement LRU cache for
    • efficient memory usage
    • preventing memory leaks by limiting the number of blocks in memory
  • ensure application scalability by enabling polycli monitor to run indefinitely without running out of memory.
  • improve performance by automatically evicting least accessed blocks, focusing resources on recent and relevant data.

added functions

  • isBlockInCache: Determines if a given block number is already available in the LRU cache to avoid unnecessary network requests.
  • checkAndFetchMissingBlocks: Identifies blocks not present in the cache within a specified range and fetches them from the Ethereum network to fill the cache.

removed

  • remove currIdx, selectedBlock, Max/MinBlockRetrieved to clean up logic variables

@gatsbyz gatsbyz force-pushed the jesse/monitor-memory-leak branch from bb7fdfd to 2ec2ef9 Compare November 3, 2023 09:05
@gatsbyz gatsbyz changed the title ensure that the ticker stops when function returns replace block data structure with LRU cache to fix memory leak Nov 6, 2023
@gatsbyz gatsbyz changed the title replace block data structure with LRU cache to fix memory leak DVT-1057 replace block data structure with LRU cache to fix memory leak Nov 6, 2023
gatsbyz
gatsbyz commented Nov 6, 2023
View reviewed changes
cmd/monitor/monitor.go
ms.BlocksLock.Lock()
ms.Blocks[pb.Number().String()] = pb
ms.BlocksLock.Unlock()
ms.BlockCache.Add(pb.Number().String(), pb)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The Add method checks if the cache is at its maximum capacity.
  • If the cache is full, it evicts the least recently used item before adding the new item.
  • If the cache is not full, it simply adds the new item to the cache.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you could add this in the code as a comment also?

@gatsbyz gatsbyz marked this pull request as ready for review November 9, 2023 13:21
leovct
leovct reviewed Nov 9, 2023
View reviewed changes
cmd/monitor/block.go Outdated Show resolved Hide resolved
leovct
leovct reviewed Nov 9, 2023
View reviewed changes
cmd/monitor/monitor.go Outdated Show resolved Hide resolved
leovct
leovct approved these changes Nov 9, 2023
View reviewed changes
Copy link
Member

@leovct leovct left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, nice job @gatsbyz!

I tested this version of polycli monitor with a very fast refresh rate while sending a lot of transactions against an anvil node and the memory looks pretty stable! 👏

Screenshot 2023-11-09 at 14 59 53 Screenshot 2023-11-09 at 14 59 47

@leovct leovct mentioned this pull request Nov 9, 2023
Merged
1 task
@gatsbyz gatsbyz merged commit 95faa01 into main Nov 13, 2023
6 checks passed
@gatsbyz gatsbyz deleted the jesse/monitor-memory-leak branch November 13, 2023 15:12
praetoriansentry added a commit that referenced this pull request Nov 13, 2023
@praetoriansentry
Revert "DVT-1057 replace block data structure with LRU cache to fix m…
54ab8c8 
…emory leak (#148)"

This reverts commit 95faa01.
@praetoriansentry praetoriansentry mentioned this pull request Nov 13, 2023
Merged
praetoriansentry added a commit that referenced this pull request Nov 13, 2023
@praetoriansentry
Revert "DVT-1057 replace block data structure with LRU cache to fix m…
f174e63 
…emory leak (#148)"

This reverts commit 95faa01.
gatsbyz added a commit that referenced this pull request Nov 14, 2023
@gatsbyz
Revert "Revert "DVT-1057 replace block data structure with LRU cache …
f06209e 
…to fix memory leak (#148)""

This reverts commit f174e63.
@gatsbyz gatsbyz mentioned this pull request Nov 14, 2023
Merged
gatsbyz added a commit that referenced this pull request Nov 14, 2023
@gatsbyz
DVT-1057 replace block data structure with LRU cache to fix memory le…
4f69f6e 
…ak (#158)

* Revert "Revert "DVT-1057 replace block data structure with LRU cache to fix memory leak (#148)""

This reverts commit f174e63.

* DVT-1057 replace block data structure with LRU cache to fix memory leak
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leovct leovct leovct approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gatsbyz @leovct